codegen: Deduplicate derive traits added by add_derives() parse callback #3296
Conversation
ivmaykov
force-pushed
the
issue-3286
branch
4 times, most recently
from
680406f to
52081ca
Compare
ivmaykov
force-pushed
the
issue-3286
branch
4 times, most recently
from
aa5e696 to
7377020
Compare
|
Is bindgen effectively unmaintained? What can I do to get this PR merged? Is this a controversial change? If so, can I have some feedback? |
|
Bindgen is a free-time project for me, this PR was opened two weeks ago. Please be patient because I unfortunately don't have a lot of free time lately :) |
bindgen/codegen/mod.rs
Outdated
| where | ||
| I: Iterator<Item = &'a str>, | ||
| { | ||
| // Use a HashSet to track already seen elements. |
There was a problem hiding this comment.
derive lists are usually pretty small, I don't think the copy + hashset overhead is going to be particularly worth it in this case...
That way the is_empty check on the caller can go. WDYT?
There was a problem hiding this comment.
Done. Performance doesn't seem to matter and the new code is simpler.
| .derive_partialord(false) | ||
| .derive_ord(false) | ||
| .derive_hash(false) | ||
| .parse_callbacks(Box::new(AddDerivesCallback::new(add_derives))) |
There was a problem hiding this comment.
Might be less code to extend bindgen-integration perhaps? But this is fine.
There was a problem hiding this comment.
I'd rather not rewrite if this works :)
|
@emilio Totally understand about the time, sorry if I came across a bit rude, wasn't my intention. Good point about hash set. I'm actually going to try benchmarking on our project. It's pretty large (generates a ~65k LoC rust file with thousands of bindings to a C library), and we actually use the add derives callback a bunch so I can see which one performs better. Unfortunately, I can't test when rebased on top of main due to #3303 which I just filed. But I can rebase on top of an older commit that doesn't have that issue and test there, will report back. |
|
BTW I'm totally cool with more people helping out with reviews and maintenance (thanks for filing that regression!) |
|
@emilio I don't think I have the bandwidth to sign up to be a maintainer, but will continue to file issues that I run across, and hopefully even fix some of them :) |
|
One particularly useful way of helping maintainers (and thus getting PRs merged) is reviewing other PRs. |
3 participants
Fixes #3286